feat(databasesql): add DSN parser registry with extensibility and tests.#509
Open
karthik120710 wants to merge 4 commits into
Open
feat(databasesql): add DSN parser registry with extensibility and tests.#509karthik120710 wants to merge 4 commits into
karthik120710 wants to merge 4 commits into
Conversation
- Added a custom DSN parser registration mechanism to support various database drivers. - Implemented a fallback parsing method for unregistered drivers, improving robustness. - Updated logging to warn when the server address cannot be determined from the DSN. - Enhanced test cases to cover new parsing logic and driver mappings. This change improves the flexibility and reliability of database connection handling. Signed-off-by: Karthik Rajan <karthikrajanmr@gmail.com>
|
|
|
The title of this pull request does not match the conventional commits format. Refer to the CONTRIBUTING.md file for more information. |
karthik120710
changed the title
|
The title of this pull request does not match the conventional commits format. Refer to the CONTRIBUTING.md file for more information. |
karthik120710
changed the title
|
The title of this pull request does not match the conventional commits format. Refer to the CONTRIBUTING.md file for more information. |
Author
|
hi @txabman42 i have added pluggable DSN parser registry and expand semconv driver mapping can u review my pr. |
|
Hey @karthik120710, nice work a few things I noticed while going through the diff Build issue: The refactor removes ParseDbName from parse.go but client.go:47 still calls it (dbName := ParseDbName(dataSourceName)). This would fail to compile. Was the removal accidental during the registry refactor? Test coverage: The new tests cover the semconv mapping changes (clickhouse, unknown driver) but there are no tests for RegisterDSNParser itself, bestEffortParse, or the new aliases like pgx and lib/pq going through parsePostgres. Since the registry is the main change here, a basic test showing registration and lookup works would help. Minor: The PR title has a leading space which is causing the conventional commit check to fail. Trimming it should fix the CI. |
karthik120710
changed the title
…verage - Refactored DSN parsing logic to utilize a dedicated internal package for improved organization and maintainability. - Renamed functions for consistency and clarity, specifically changing to . - Added comprehensive unit tests for the new parsing logic, ensuring robust handling of various DSN formats and edge cases. This update enhances the overall structure of the codebase and improves the reliability of database connection handling. Signed-off-by: Karthik Rajan <karthikrajanmr@gmail.com>
Author
|
Hi @waqar2403 I have add units test and updated process title , but still unit test are not running . |
|
@karthik120710 good progress on the registry refactoring. Two things here: Tests not running Your go.mod bumps a few OTel indirect deps (otlploghttp, otel/log, sdk/log) but the go.sum likely needs regeneration. Try this from the module root: If there are still import resolution errors after that, check if testify is listed under Minor style note on RegisterDSNParse Right now var RegisterDSNParser = dsnparse.RegisterDSNParserThis means any consumer can accidentally overwrite it like func RegisterDSNParser(driverName string, parser dsnparse.DSNParser) {
dsnparse.RegisterDSNParser(driverName, parser)
}Not a blocker just something to consider for safety. |
- Introduced a wrapper function for to maintain consistency in the registration process. - This change enhances the clarity of the parser registration mechanism while preserving existing functionality. This update contributes to better code organization and maintainability in the DSN parsing logic. Signed-off-by: Karthik Rajan <karthikrajanmr@gmail.com>
📊 Semantic Convention Registry Update CheckCurrent Project Version:
🆕 Available UpdatesComparing latest (main branch) vs v1.37.0 (current) 💡 New semantic conventions are available. Consider updating if these changes are relevant to your instrumentation: 📋 View available updatesSummary of Schema ChangesRegistry versionsBaseline: unversioned Head: unversioned Registry AttributesAdded
Renamed
Obsoleted
Uncategorized
MetricsAdded
Renamed
Obsoleted
Uncategorized
EventsAdded
Obsoleted
Uncategorized
📝 What This Means
How to Update (if desired):
Generated by OTel Weaver • Run |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #509 +/- ##
=======================================
Coverage ? 63.30%
=======================================
Files ? 62
Lines ? 4796
Branches ? 0
=======================================
Hits ? 3036
Misses ? 1509
Partials ? 251
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kakkoyun
requested changes
May 19, 2026
Member
kakkoyun
left a comment
kakkoyun
left a comment
There was a problem hiding this comment.
Thanks for the initiative.
This requires a bit of improvement. Please see my comments.
| ) | ||
|
|
||
| // DSNParser parses a driver-specific DSN and returns the server address (host:port). | ||
| type DSNParser func(dsn string) (addr string, err error) |
Member
There was a problem hiding this comment.
I think I would prefer having an interface for parser, and implementations in struct.
|
|
||
| var ( | ||
| parserMu sync.RWMutex | ||
| parserRegistry = map[string]DSNParser{} |
Member
There was a problem hiding this comment.
We can have another struct type for registry and we don't need package level locks.
|
|
||
| func init() { | ||
| // Register all built-in DSN parsers. | ||
| RegisterDSNParser("mysql", ParseMySQL) |
Member
There was a problem hiding this comment.
We are constantly locking and unlocking here. We can have a batch API where lock only once.
|
|
||
| // BestEffortParse attempts to extract a host:port from a DSN using standard URL parsing. | ||
| // It is used as a fallback for drivers that have no registered parser. | ||
| func BestEffortParse(dsn string) (string, error) { |
Member
There was a problem hiding this comment.
We don't need to export this method.
| // clickhouse://host:port/database?username=user&password=pass | ||
| u, err := nurl.Parse(dsn) | ||
| if err != nil { | ||
| return "", err |
Member
There was a problem hiding this comment.
We shouldn't just return the net/url error here. It exposes the rawURL and it means we can leak the sensitive information to some external service if somebody tries to log or send the error.
See: https://cs.opensource.google/go/go/+/refs/tags/go1.26.3:src/net/url/url.go;l=398
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
without modifying core code.
init()).
fell through to other_sql.
Test plan
DSN parser lacks extensibility and silently produces empty addresses for unknown drivers #508